Skip to content

gh-102058: Update descriptor.rst#102059

Closed
rasputyashka wants to merge 3 commits into
python:mainfrom
rasputyashka:fix-issue-102058
Closed

gh-102058: Update descriptor.rst#102059
rasputyashka wants to merge 3 commits into
python:mainfrom
rasputyashka:fix-issue-102058

Conversation

@rasputyashka
Copy link
Copy Markdown

@rasputyashka rasputyashka commented Feb 19, 2023

@ghost
Copy link
Copy Markdown

ghost commented Feb 19, 2023

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-bot bedevere-bot added awaiting review docs Documentation in the Doc dir skip news labels Feb 19, 2023
@Eclips4
Copy link
Copy Markdown
Member

Eclips4 commented Feb 19, 2023

You should use double backticks.
Otherwise, looks good to me.

@rasputyashka rasputyashka changed the title Update descriptor.rst gh-102058: Update descriptor.rst Feb 19, 2023
Comment thread Doc/howto/descriptor.rst Outdated
Added replaced * with note

Co-authored-by: Eclips4 <80244920+Eclips4@users.noreply.github.com>
@hauntsaninja
Copy link
Copy Markdown
Contributor

Thanks, but I don't think this full paragraph warning is necessary. In this ORM example, the attribute names and owners come from the class definition, and are not program inputs (which is where the risk of SQL injection would come from).

The warning is unrelated to descriptors, so IMO this mostly just serves as a distraction. Descriptors are a relatively advanced concept; I think we can trust our readers here to determine for themselves when SQL injection is a risk.

@erlend-aasland
Copy link
Copy Markdown
Contributor

I agree, @hauntsaninja. Also, the interpolated values are used in places where SQLite placeholders are not applicable; for example, you cannot use placeholders to chose which table you want to query.

If there is one enhancement we could do here, it might be to link to the sqlite3 placeholders how-to.

@Eclips4: please see the devguide for our review practices.

@Eclips4
Copy link
Copy Markdown
Member

Eclips4 commented Feb 20, 2023

Thanks @erlend-aasland & @hauntsaninja for answer!
Can we write something like that?

In this example we inventionally didn't use placeholders (see :ref:`sqlite3-placeholders` for more details).

@rasputyashka
Copy link
Copy Markdown
Author

rasputyashka commented Feb 20, 2023

in fact, we did:) @Eclips4

@rasputyashka
Copy link
Copy Markdown
Author

rasputyashka commented Feb 20, 2023

@erlend-aasland As I said, if you use string formatting, you should check if data in a string is correct (i'd say valid) i.e. check if a column or table with such name exists. I guess link to sqlite3 placeholders's how-to in our case would be enough to dispel my concerns about f-strings. I am not actually sure whether the problem I mentioned earlier (about validating data) need to be solved, so I'd just leave a link as eclips4 did.

I propose to write:

see :ref:`sqlite3-placehoders` for more information about use of placeholders in sqlite3

If we're ok with that, i'll commit the changes.

@rhettinger
Copy link
Copy Markdown
Contributor

rhettinger commented Feb 20, 2023

As the other commenters noted, this editorializing is a distractor from showing how descriptors work. Also, there is no placeholder solution to choosing the table name.

Thanks for the suggestion, but I am going to decline this PR.

@rhettinger rhettinger closed this Feb 20, 2023
@rasputyashka rasputyashka deleted the fix-issue-102058 branch February 20, 2023 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting core review docs Documentation in the Doc dir skip news

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Descriptor HOWTO: usage of f-strings in sql queries example

6 participants